[https://nvbugs/6306936][test] Re-enable AutoDeploy disagg tests#15325
Conversation
|
/bot run --disable-fail-fast |
📝 WalkthroughWalkthroughThis PR consolidates B300/GB300 test skip logic from the waives configuration file into pytest autouse fixtures within the test modules themselves. Two disaggregated AutoDeploy test files gain ChangesB300/GB300 test skip consolidation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/integration/defs/disaggregated/test_ad_disagg.py (1)
45-46: 📐 Maintainability & Code Quality | ⚡ Quick winAdd explicit
-> Noneto the new autouse fixtures.Both new fixture definitions (Line 45 in
tests/integration/defs/disaggregated/test_ad_disagg.pyand Line 38 intests/integration/defs/disaggregated/test_ad_disagg_trtllm_serve.py) should declare-> Noneto match the repository’s Python typing guideline for function annotations.As per coding guidelines, “Always annotate functions. Make the return type
Noneif the function does not return anything.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration/defs/disaggregated/test_ad_disagg.py` around lines 45 - 46, The autouse fixture definitions need explicit return annotations; update the fixture signature for skip_b300 (def skip_b300()) to include -> None (def skip_b300() -> None:) and do the same for the other new autouse fixture in tests/integration/defs/disaggregated/test_ad_disagg_trtllm_serve.py so both functions are annotated with return type None to comply with the repository typing guideline.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/integration/defs/disaggregated/test_ad_disagg.py`:
- Around line 45-46: The autouse fixture definitions need explicit return
annotations; update the fixture signature for skip_b300 (def skip_b300()) to
include -> None (def skip_b300() -> None:) and do the same for the other new
autouse fixture in
tests/integration/defs/disaggregated/test_ad_disagg_trtllm_serve.py so both
functions are annotated with return type None to comply with the repository
typing guideline.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 94aa4fd5-8a7a-44df-b36f-12d97a4e85c4
📒 Files selected for processing (3)
tests/integration/defs/disaggregated/test_ad_disagg.pytests/integration/defs/disaggregated/test_ad_disagg_trtllm_serve.pytests/integration/test_lists/waives.txt
💤 Files with no reviewable changes (1)
- tests/integration/test_lists/waives.txt
govind-ramnarayan
changed the title
|
PR_Github #53966 [ run ] triggered by Bot. Commit: |
govind-ramnarayan
changed the title
|
PR_Github #53966 [ run ] completed with state
|
|
/bot run |
|
PR_Github #54152 [ run ] triggered by Bot. Commit: |
|
PR_Github #54152 [ run ] completed with state
|
|
/bot run --stage-list "A30-AutoDeploy-1" |
|
PR_Github #54357 [ run ] triggered by Bot. Commit: |
|
PR_Github #54357 [ run ] completed with state
|
govind-ramnarayan
force-pushed
the
gramnarayan/undo-disagg-waives
branch
from
accdbaf to
d66f329
Compare
|
/bot run --disable-fail-fast |
|
PR_Github #54671 [ run ] triggered by Bot. Commit: |
|
/bot run --disable-fail-fast |
|
PR_Github #54681 [ run ] triggered by Bot. Commit: |
|
PR_Github #54671 [ run ] completed with state |
|
PR_Github #54681 [ run ] completed with state
|
|
/bot run |
Signed-off-by: Govind Ramnarayan <105831528+govind-ramnarayan@users.noreply.github.com>
Signed-off-by: Govind Ramnarayan <105831528+govind-ramnarayan@users.noreply.github.com>
Signed-off-by: Govind Ramnarayan <105831528+govind-ramnarayan@users.noreply.github.com>
govind-ramnarayan
force-pushed
the
gramnarayan/undo-disagg-waives
branch
from
fb3b06b to
3e2cc43
Compare
|
/bot run |
|
PR_Github #55066 [ run ] triggered by Bot. Commit: |
…m factory. Maybe we want better defaults in the future but production config files are agent-generated so they dont have this issue when they set max_seq_len Signed-off-by: Govind Ramnarayan <105831528+govind-ramnarayan@users.noreply.github.com>
|
/bot kill |
|
PR_Github #55082 [ kill ] triggered by Bot. Commit: |
|
/bot run --stage-list "A30-AutoDeploy-1" |
|
PR_Github #55066 [ run ] completed with state |
|
PR_Github #55082 [ kill ] completed with state |
|
PR_Github #55083 [ run ] triggered by Bot. Commit: |
|
PR_Github #55083 [ run ] completed with state |
|
/bot run |
|
PR_Github #55092 [ run ] triggered by Bot. Commit: |
|
PR_Github #55092 [ run ] completed with state |
|
/bot run --post-merge |
|
PR_Github #55168 [ run ] triggered by Bot. Commit: |
|
PR_Github #55168 [ run ] completed with state |
Signed-off-by: Gal Hubara-Agam <96368689+galagam@users.noreply.github.com>
|
/bot run --post-merge |
|
PR_Github #55208 [ run ] triggered by Bot. Commit: |
|
PR_Github #55208 [ run ] completed with state |
…DIA#15325) Signed-off-by: Govind Ramnarayan <105831528+govind-ramnarayan@users.noreply.github.com> Signed-off-by: Gal Hubara-Agam <96368689+galagam@users.noreply.github.com> Co-authored-by: Gal Hubara-Agam <96368689+galagam@users.noreply.github.com> Signed-off-by: GitLab CI Bot <gitlab-ci@nvidia.com>
5 participants
#15260 should fix all AutoDeploy disagg tests by resolving a merge-time conflict between tests.
This follow up is to unwaive all disagg tests and smoke tests waived due to the above-mentioned AutoDeploy disagg failure.
Additionally, re-enabling the tests may cause issues on GB300 according to the nvbug filed here: https://nvbugspro.nvidia.com/bug/6301621 so we are disabling these tests on GB300 until we are able to test manually and debug.
@coderabbitai summary
Description
Test Coverage
PR Checklist
Please review the following before submitting your PR:
PR description clearly explains what and why. If using CodeRabbit's summary, please make sure it makes sense.
PR Follows TRT-LLM CODING GUIDELINES to the best of your knowledge.
Test cases are provided for new code paths (see test instructions)
If PR introduces API changes, an appropriate PR label is added - either
api-compatibleorapi-breaking. Forapi-breaking, includeBREAKINGin the PR title.Any new dependencies have been scanned for license and vulnerabilities
CODEOWNERS updated if ownership changes
Documentation updated as needed
Update tava architecture diagram if there is a significant design change in PR.
The reviewers assigned automatically/manually are appropriate for the PR.
Please check this after reviewing the above items as appropriate for this PR.
GitHub Bot Help
To see a list of available CI bot commands, please comment
/bot help.